Skip to content

[659] feat(form): Add reCAPTCHA validation to form submissions - #197

Merged
killev merged 31 commits into
mainfrom
659-admin-recaptcha-config
Jul 8, 2025
Merged

[659] feat(form): Add reCAPTCHA validation to form submissions#197
killev merged 31 commits into
mainfrom
659-admin-recaptcha-config

Conversation

@IhorMasechko

@IhorMasechko IhorMasechko commented Jul 3, 2025

Copy link
Copy Markdown
Contributor
  • add server-side reCAPTCHA verification to form submission route
  • add client-side reCAPTCHA validation and error handling
  • update form widget template to include reCAPTCHA widget and error message
  • update styles for reCAPTCHA and error messages
  • add abort-controller and node-fetch dependencies for server-side validation

@IhorMasechko IhorMasechko self-assigned this Jul 3, 2025
@IhorMasechko
IhorMasechko requested a review from killev as a code owner July 3, 2025 09:28
@coderabbitai

coderabbitai Bot commented Jul 3, 2025

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This change introduces Google reCAPTCHA validation to the website's form handling. It updates the form widget template, adds server-side and client-side reCAPTCHA verification logic, integrates validation schemas, adjusts form error handling and styling, and includes new modules and tests for reCAPTCHA validation and error management. Dependencies for HTTP requests and abort handling are added.

Changes

File(s) Change Summary
website/modules/@apostrophecms/form-widget/views/widget.html Updated template to render reCAPTCHA widget and error message, removed conditional disabling of submit button, and revised error handling.
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html New template added to include Google reCAPTCHA API script asynchronously.
website/modules/@apostrophecms/form/index.js Refactored POST route handler; added server-side reCAPTCHA verification using new utility.
website/modules/@apostrophecms/form/lib/verifyRecaptcha.js New module: Implements reCAPTCHA token verification with Google API and timeout handling.
website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js New test suite: Tests verifyRecaptcha under various scenarios using mocks.
website/modules/@apostrophecms/form/lib/formatForSpreadsheet.js Excludes 'g-recaptcha-response' from spreadsheet export headers and row data.
website/modules/asset/ui/src/js/validationSchemas.js Added validation schema for 'g-recaptcha-response' field.
website/modules/asset/ui/src/js/formValidation.js Integrated client-side reCAPTCHA validation and error handling into form submission logic.
website/modules/asset/ui/src/js/recaptchaValidation.js New module: Handles reCAPTCHA value change detection and error clearing in forms.
website/modules/asset/ui/src/js/recaptchaValidation.test.js New test suite: Verifies handleRecaptchaValueChange logic in DOM.
website/modules/asset/ui/src/scss/_form.scss Adjusted form and error message positioning, padding, and consolidated error styles for reCAPTCHA integration.
website/package.json Added dependencies: abort-controller and node-fetch for server-side HTTP requests and timeout support.
sonar-project.properties Added new file exclusion for reCAPTCHA script template to SonarQube exclusions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Browser
    participant FormWidget (Client)
    participant Server
    participant Google reCAPTCHA

    User->>Browser: Loads form page
    Browser->>FormWidget: Renders form with reCAPTCHA widget
    User->>FormWidget: Fills form, completes reCAPTCHA
    FormWidget->>Browser: Validates form and reCAPTCHA (client-side)
    alt reCAPTCHA incomplete
        FormWidget->>User: Shows reCAPTCHA error, blocks submit
    else reCAPTCHA complete
        FormWidget->>Server: Submits form with reCAPTCHA token
        Server->>Google reCAPTCHA: Verifies token (server-side)
        alt Verification fails
            Server->>FormWidget: Returns error, blocks submission
        else Verification succeeds
            Server->>FormWidget: Processes form, returns success or other errors
        end
    end
Loading

Suggested reviewers

  • IhorMasechko
  • Anton-88
  • killev

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions

github-actions Bot commented Jul 3, 2025

Copy link
Copy Markdown

🔍 Vulnerabilities of apostrophe-cms:test

📦 Image Reference apostrophe-cms:test
digestsha256:6200c2d02e9e9930869beac9577c6e67d3b121d6405c92eb80b792b88744f73c
vulnerabilitiescritical: 0 high: 3 medium: 0 low: 0
platformlinux/amd64
size291 MB
packages984
📦 Base Image node:23-alpine
also known as
  • 23-alpine3.22
  • 23.11-alpine
  • 23.11-alpine3.22
  • 23.11.1-alpine
  • 23.11.1-alpine3.22
digestsha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd
vulnerabilitiescritical: 0 high: 0 medium: 1 low: 1
critical: 0 high: 1 medium: 0 low: 0 async 0.9.2 (npm)

pkg:npm/async@0.9.2

high 7.8: CVE--2021--43138 OWASP Top Ten 2017 Category A9 - Using Components with Known Vulnerabilities

Affected range<2.6.4
Fixed version2.6.4, 3.2.2
CVSS Score7.8
CVSS VectorCVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H
EPSS Score0.907%
EPSS Percentile75th percentile
Description

A vulnerability exists in Async through 3.2.1 (fixed in 3.2.2), which could let a malicious user obtain privileges via the mapValues() method.

critical: 0 high: 1 medium: 0 low: 0 connect-multiparty 2.2.0 (npm)

pkg:npm/connect-multiparty@2.2.0

high 7.8: CVE--2022--29623 Unrestricted Upload of File with Dangerous Type

Affected range<=2.2.0
Fixed versionNot Fixed
CVSS Score7.8
CVSS VectorCVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H
EPSS Score0.334%
EPSS Percentile56th percentile
Description

An arbitrary file upload vulnerability in the file upload module of Express Connect-Multiparty 2.2.0 allows attackers to execute arbitrary code via a crafted PDF file. NOTE: the Supplier has not verified this vulnerability report.

critical: 0 high: 1 medium: 0 low: 0 async 1.5.2 (npm)

pkg:npm/async@1.5.2

high 7.8: CVE--2021--43138 OWASP Top Ten 2017 Category A9 - Using Components with Known Vulnerabilities

Affected range<2.6.4
Fixed version2.6.4, 3.2.2
CVSS Score7.8
CVSS VectorCVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H
EPSS Score0.907%
EPSS Percentile75th percentile
Description

A vulnerability exists in Async through 3.2.1 (fixed in 3.2.2), which could let a malicious user obtain privileges via the mapValues() method.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
website/modules/custom-form/index.js (1)

7-11: Consider adding parameter validation.

The submit handler should validate the req parameter to ensure it has the expected structure before accessing nested properties.

 async submit(req) {
+  if (!req || !req.apos || !req.body) {
+    return req.res.status(400).json({ error: 'Invalid request structure.' });
+  }
   const global = await req.apos.global.get(req);
   const recaptchaToken = req.body['g-recaptcha-response'];
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe825f6 and 2d92e5d.

📒 Files selected for processing (3)
  • website/app.js (1 hunks)
  • website/modules/@apostrophecms/form-widget/views/widget.html (1 hunks)
  • website/modules/custom-form/index.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{js,jsx}`: Use 2 spaces for indentation Maximum line length: 80 characters...

**/*.{js,jsx}: Use 2 spaces for indentation
Maximum line length: 80 characters
Use semicolons at the end of statements
Use single quotes for strings
Add trailing commas in arrays, objects, etc.
No trailing spaces
Use consistent quote properties (either quote all properties or none)
Place line comments above code, not inline
Capitalize all comments
No inline comments
Maximum function length enforced (avoid excessive length)
Maximum depth: 4 levels
Maximum callback nesting: 3 levels
Maximum parameters: 5
Maximum statements per function: 50
Use function declarations with named functions, not function expressions
Always initialize variables at declaration
Maximum lines per file: 300
Use destructuring where possible
Minimum identifier length enforced (no single-letter variables)
No unused variables
No reassignment of function parameters
No invalid 'this' context
No duplicate object keys
No ternary operators (use if/else)
Maximum complexity: 15 (cognitive complexity)
No alerts or console logs
No debugger statements
No identical expressions in conditions
Use optimized regex patterns
Use Unicode regex patterns
No secrets in code
No unsanitized methods or properties (XSS prevention)
Sort imports alphabetically
No unresolved imports
No importing default from a module that doesn't have a default export
Always return in promise chains

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/app.js
  • website/modules/custom-form/index.js
`**/*.js`: No missing imports in Node.js No missing require statements

**/*.js: No missing imports in Node.js
No missing require statements

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/app.js
  • website/modules/custom-form/index.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/app.js (1)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
website/modules/@apostrophecms/form-widget/views/widget.html (3)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/modules/custom-form/index.js (2)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: e2e-tests
  • GitHub Check: unit-tests
  • GitHub Check: lint
  • GitHub Check: security-scan
🔇 Additional comments (12)
website/app.js (1)

115-115: LGTM! Module integration looks correct.

The addition of the custom-form module to the configuration follows ApostropheCMS conventions and properly integrates the new reCAPTCHA functionality.

website/modules/custom-form/index.js (4)

3-4: LGTM! Module structure follows ApostropheCMS conventions.

The module correctly extends the @apostrophecms/form module using the standard extension pattern.


13-16: LGTM! Proper validation of reCAPTCHA token presence.

The conditional check for reCAPTCHA enablement and token presence is well-implemented with appropriate error response.


44-44: LGTM! Proper delegation to parent handler.

The fallback to the original Apostrophe form submit handler is correctly implemented.


1-1: Verify node-fetch is declared and installed

I wasn’t able to locate a package.json or node_modules directory in the workspace. Please confirm that node-fetch is listed under dependencies in your project’s manifest and installed, so that

// website/modules/custom-form/index.js (line 1)
const fetch = require('node-fetch');

will resolve correctly at runtime.

• Check your package.json (or equivalent) for "node-fetch" under dependencies
• If it’s missing, run npm install node-fetch (or the appropriate package manager command)

website/modules/@apostrophecms/form-widget/views/widget.html (7)

12-14: LGTM! Template structure reorganization improves readability.

The reorganization of the form parameters logic maintains functionality while improving code structure.


16-22: LGTM! Form attributes are properly configured.

The form element has appropriate attributes for security and functionality, including proper action URL and form validation settings.


23-23: LGTM! Form contents area placement is logical.

Moving the form contents area before the reCAPTCHA widget and submit button creates a more intuitive form flow.


25-30: LGTM! Conditional reCAPTCHA rendering is properly implemented.

The conditional rendering of the reCAPTCHA script and widget aligns with the backend validation logic and follows best practices.


32-35: LGTM! Submit button is properly configured.

The submit button is correctly implemented without conditional disabling, allowing for proper form submission flow.


37-55: LGTM! Error messages are properly structured with accessibility attributes.

The error message containers have appropriate role="alert" attributes and conditional rendering, ensuring good accessibility and user experience.


57-73: LGTM! Spinner and thank-you messages are well-organized.

The spinner and thank-you message sections are properly structured with appropriate classes and accessibility attributes.

Comment thread website/modules/custom-form/index.js Outdated
Comment thread website/modules/custom-form/index.js Outdated
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 3, 2025
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 3, 2025
- Add verifyRecaptcha utility and tests for Google reCAPTCHA validation
- Integrate reCAPTCHA check in form module submit route
- Update validationSchemas to require and validate reCAPTCHA token

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
website/modules/@apostrophecms/form/index.js (1)

25-36: Optimize global document retrieval and improve error handling.

The global document is retrieved on every request, which could impact performance. Consider caching or checking if reCAPTCHA is needed before retrieval.

- const globalDoc = await self.apos.global.find(req).toObject();
  const recaptchaToken = formData['g-recaptcha-response'];
- if (globalDoc.useRecaptcha && globalDoc.recaptchaSecret) {
+ if (recaptchaToken) {
+   const globalDoc = await self.apos.global.find(req).toObject();
+   if (globalDoc.useRecaptcha && globalDoc.recaptchaSecret) {
     const result = await verifyRecaptcha({
       secret: globalDoc.recaptchaSecret,
       token: recaptchaToken,
       remoteip: req.ip,
     });
     if (!result.success) {
       return res.status(400).json({ error: result.error });
     }
+   } else if (globalDoc.useRecaptcha) {
+     return res.status(500).json({ error: 'reCAPTCHA configuration error' });
+   }
   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 431b416 and f68f03e.

📒 Files selected for processing (4)
  • website/modules/@apostrophecms/form/index.js (3 hunks)
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.js (1 hunks)
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js (1 hunks)
  • website/modules/asset/ui/src/js/validationSchemas.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.{js,jsx}`: Use 2 spaces for indentation Maximum line length: 80 characters...

**/*.{js,jsx}: Use 2 spaces for indentation
Maximum line length: 80 characters
Use semicolons at the end of statements
Use single quotes for strings
Add trailing commas in arrays, objects, etc.
No trailing spaces
Use consistent quote properties (either quote all properties or none)
Place line comments above code, not inline
Capitalize all comments
No inline comments
Maximum function length enforced (avoid excessive length)
Maximum depth: 4 levels
Maximum callback nesting: 3 levels
Maximum parameters: 5
Maximum statements per function: 50
Use function declarations with named functions, not function expressions
Always initialize variables at declaration
Maximum lines per file: 300
Use destructuring where possible
Minimum identifier length enforced (no single-letter variables)
No unused variables
No reassignment of function parameters
No invalid 'this' context
No duplicate object keys
No ternary operators (use if/else)
Maximum complexity: 15 (cognitive complexity)
No alerts or console logs
No debugger statements
No identical expressions in conditions
Use optimized regex patterns
Use Unicode regex patterns
No secrets in code
No unsanitized methods or properties (XSS prevention)
Sort imports alphabetically
No unresolved imports
No importing default from a module that doesn't have a default export
Always return in promise chains

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/validationSchemas.js
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js
  • website/modules/@apostrophecms/form/index.js
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.js
`**/*.js`: No missing imports in Node.js No missing require statements

**/*.js: No missing imports in Node.js
No missing require statements

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/validationSchemas.js
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js
  • website/modules/@apostrophecms/form/index.js
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.js
`**/*.test.{js,jsx}`: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring

**/*.test.{js,jsx}: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js (2)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.test.{js,jsx} : Test files have relaxed rules for function length, statements, extraneous requires, and destructuring
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.e2e.{js,jsx} : E2E test files: relaxed import, ternary, and sorting rules
website/modules/@apostrophecms/form/index.js (2)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
🧬 Code Graph Analysis (1)
website/modules/@apostrophecms/form/index.js (1)
website/modules/@apostrophecms/form/lib/verifyRecaptcha.js (1)
  • verifyRecaptcha (3-34)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: lint
  • GitHub Check: e2e-tests
  • GitHub Check: unit-tests
  • GitHub Check: security-scan
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js (1)

1-62: Excellent test coverage and implementation!

The test suite comprehensively covers all scenarios:

  • Missing token validation
  • Failed verification responses
  • Successful verification
  • HTTP error handling

The mocking approach is appropriate and the test structure follows best practices.

website/modules/@apostrophecms/form/lib/verifyRecaptcha.js (1)

25-25: Add error handling for JSON parsing.

The response.json() call can throw an error if the response is not valid JSON.

- const data = await response.json();
+ let data;
+ try {
+   data = await response.json();
+ } catch (error) {
+   return { success: false, error: 'Invalid response from reCAPTCHA service.' };
+ }
⛔ Skipped due to learnings
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
website/modules/@apostrophecms/form/index.js (1)

17-49: LGTM! Well-structured reCAPTCHA integration.

The extraction of the inline handler to a named function significantly improves code organization and readability. The reCAPTCHA verification logic is properly integrated with appropriate error handling and status codes.

The error handling follows the established pattern from previous learnings, allowing errors to bubble up to higher-level handlers.

Comment thread website/modules/asset/ui/src/js/validationSchemas.js Outdated
Comment thread website/modules/@apostrophecms/form/lib/verifyRecaptcha.js Outdated
Comment thread website/modules/@apostrophecms/form/lib/verifyRecaptcha.js Outdated
Comment thread website/modules/@apostrophecms/form/index.js Outdated
VitalyyP added 2 commits July 4, 2025 11:11
- Exclude 'g-recaptcha-response' from headers and row data in formatForSpreadsheet
- Update header and row generation logic to filter out recaptcha field
- Change node-fetch version from ^3.3.2 to ^2.7.0 in package.json
- Update package-lock.json to reflect dependency change
@VitalyyP VitalyyP self-assigned this Jul 4, 2025
VitalyyP added 2 commits July 4, 2025 13:23
- Delete website/modules/custom-form/index.js
- Remove 'custom-form' from modules list in website/app.js
- Refactor reCAPTCHA error message placement in widget.html
- Add client-side reCAPTCHA validation to formValidation.js
- Update SCSS for new captcha error styles and positioning

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
website/modules/asset/ui/src/js/formValidation.js (2)

213-213: Remove unnecessary empty line.

This empty line after event.preventDefault(); appears unnecessary and doesn't follow the coding style.

-  event.preventDefault();
-
+  event.preventDefault();

216-233: Add error handling for reCAPTCHA script loading failures.

The reCAPTCHA validation logic is well-implemented, but it assumes the reCAPTCHA script loads successfully. Consider adding error handling for cases where the script fails to load or the widget isn't properly initialized.

  // ReCAPTCHA validation (client-side)
  const recaptchaWidget = form.querySelector('.g-recaptcha');
  const recaptchaError = document.querySelector(
    '[data-apos-form-recaptcha-error]',
  );
  if (
-    typeof window.grecaptcha !== 'undefined' &&
+    typeof window.grecaptcha !== 'undefined' &&
+    typeof window.grecaptcha.getResponse === 'function' &&
    recaptchaWidget &&
    !window.grecaptcha.getResponse()
  ) {
website/modules/asset/ui/src/scss/_form.scss (2)

55-58: Consider making the reCAPTCHA error positioning more responsive.

The fixed left: 129px value appears to be a magic number that might not work well across different screen sizes or reCAPTCHA widget variations.

Consider using a more flexible positioning approach:

  .apos-form-captcha-error {
    bottom: 0px;
-   left: 129px;
+   left: 50%;
+   transform: translateX(-50%);
+   
+   @include breakpoint-medium {
+     left: 129px;
+     transform: none;
+   }
  }

179-194: Clean up commented-out code.

The consolidated error message styles look good, but the commented-out positioning code should be removed to keep the stylesheet clean.

  .validation-error,
  .apos-form-error,
  .apos-form-captcha-error {
    position: absolute;
-   // bottom: -4px;
-   // left: 5px;
-   // @include breakpoint-medium {
-   //   left: 25px;
-   // }
    color: $error-color;
    font-size: 12px;
    margin-top: -5px;
    margin-bottom: 10px;
    font-weight: 500;
    line-height: 110%;
  }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d73488 and 867d8e7.

📒 Files selected for processing (3)
  • website/modules/@apostrophecms/form-widget/views/widget.html (1 hunks)
  • website/modules/asset/ui/src/js/formValidation.js (1 hunks)
  • website/modules/asset/ui/src/scss/_form.scss (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • website/modules/@apostrophecms/form-widget/views/widget.html
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{js,jsx}`: Use 2 spaces for indentation Maximum line length: 80 characters...

**/*.{js,jsx}: Use 2 spaces for indentation
Maximum line length: 80 characters
Use semicolons at the end of statements
Use single quotes for strings
Add trailing commas in arrays, objects, etc.
No trailing spaces
Use consistent quote properties (either quote all properties or none)
Place line comments above code, not inline
Capitalize all comments
No inline comments
Maximum function length enforced (avoid excessive length)
Maximum depth: 4 levels
Maximum callback nesting: 3 levels
Maximum parameters: 5
Maximum statements per function: 50
Use function declarations with named functions, not function expressions
Always initialize variables at declaration
Maximum lines per file: 300
Use destructuring where possible
Minimum identifier length enforced (no single-letter variables)
No unused variables
No reassignment of function parameters
No invalid 'this' context
No duplicate object keys
No ternary operators (use if/else)
Maximum complexity: 15 (cognitive complexity)
No alerts or console logs
No debugger statements
No identical expressions in conditions
Use optimized regex patterns
Use Unicode regex patterns
No secrets in code
No unsanitized methods or properties (XSS prevention)
Sort imports alphabetically
No unresolved imports
No importing default from a module that doesn't have a default export
Always return in promise chains

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/formValidation.js
`**/*.js`: No missing imports in Node.js No missing require statements

**/*.js: No missing imports in Node.js
No missing require statements

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/formValidation.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/modules/asset/ui/src/scss/_form.scss (7)
Learnt from: yuramax
PR: speedandfunction/website#121
File: website/modules/asset/ui/src/scss/_vacancies.scss:99-102
Timestamp: 2025-05-26T15:57:01.766Z
Learning: In website/modules/asset/ui/src/scss/_vacancies.scss, the margin shorthand (e.g., `margin: 0 0 8px`) in `.sf-vacancies_footer h2` is intentionally used to reset all margins and set specific bottom margins, rather than using `margin-bottom` alone. This approach ensures clean baseline styling that doesn't inherit unwanted margins, while still allowing the sibling selector `.vacancy-card + .sf-vacancies_footer h2` to add top margins when a vacancy card precedes the footer.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:10.045Z
Learning: In the website project, the `.sf-form` CSS class selector is intentionally used in form validation because it's the class used in the form widget template, not an inconsistency that needs to be fixed.
Learnt from: IhorMasechko
PR: speedandfunction/website#132
File: website/modules/asset/ui/src/scss/_not-found.scss:52-64
Timestamp: 2025-05-29T07:16:52.843Z
Learning: In website/modules/asset/ui/src/scss/_not-found.scss, the .two-buttons container with flex-direction: row and child .sf-button elements having width: 100% does not cause overflow issues and renders correctly, despite theoretical expectations.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/index.js : modules/asset/ui/src/index.js: relaxed function lines and style rules
Learnt from: yuramax
PR: speedandfunction/website#191
File: website/modules/asset/ui/src/scss/_leadership-team.scss:213-216
Timestamp: 2025-07-02T08:15:36.152Z
Learning: In website/modules/asset/ui/src/scss/_leadership-team.scss, the `.leader-bio` element intentionally uses `overflow-y: hidden` at the `breakpoint-extra-large` level to completely remove scrollbars on desktop devices, while maintaining `overflow-y: auto` at the `breakpoint-large` level for tablets. This design decision relies on content managers controlling bio text length to prevent overflow on desktop, ensuring a clean scrollbar-free experience on larger screens.
website/modules/asset/ui/src/js/formValidation.js (1)
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: lint
  • GitHub Check: e2e-tests
  • GitHub Check: unit-tests
  • GitHub Check: security-scan
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
website/modules/asset/ui/src/js/formValidation.js (2)

242-250: Verify the promise chain logic for combined validation.

The logic correctly combines reCAPTCHA validation with form validation using the hasError flag. However, ensure that all error states are properly handled and that the form submission is prevented when either validation fails.

The implementation correctly:

  • Combines reCAPTCHA and form validation results
  • Prevents submission when either validation fails
  • Maintains the existing validation flow

256-257: Return statement placement is correct.

The function correctly returns true immediately after starting the asynchronous validation process, which prevents the default form submission behavior while allowing the validation to complete asynchronously.

website/modules/asset/ui/src/scss/_form.scss (2)

2-2: Position relative is necessary for absolute positioning of child elements.

Adding position: relative to the container is correct and necessary for the absolute positioning of error messages implemented later in the file.


37-40: reCAPTCHA widget styling is appropriate.

The relative positioning and top margin for the .g-recaptcha widget provides good visual separation and alignment with the form elements.

VitalyyP added 2 commits July 4, 2025 18:21
- Refactor reCAPTCHA error message markup in widget.html for clarity and structure
- Adjust .apos-form-captcha-error left position and add responsive breakpoint in _form.scss
- Add recaptchaValidation.js for reCAPTCHA field event handling
- Integrate addRecaptchaValidationHandlers in formValidation.js
- Add unit tests for handleRecaptchaValueChange

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
website/modules/asset/ui/src/js/recaptchaValidation.test.js (1)

1-42: Test coverage is good but could be enhanced with additional edge cases.

The test suite effectively covers the main scenarios for handleRecaptchaValueChange. The setup is well-structured with proper DOM mocking and cleanup.

Consider adding these additional test cases for better coverage:

+  it('handles missing error element gracefully', () => {
+    errorMsg.remove();
+    recaptchaTextarea.value = 'token';
+    expect(() => {
+      handleRecaptchaValueChange(recaptchaTextarea, form, clearValidationErrorFn);
+    }).not.toThrow();
+    expect(clearValidationErrorFn).toHaveBeenCalledWith(recaptchaTextarea);
+  });
+
+  it('handles falsy values correctly', () => {
+    errorMsg.classList.remove('apos-form-hidden');
+    recaptchaTextarea.value = null;
+    handleRecaptchaValueChange(recaptchaTextarea, form, clearValidationErrorFn);
+    expect(clearValidationErrorFn).not.toHaveBeenCalled();
+    expect(errorMsg.classList.contains('apos-form-hidden')).toBe(false);
+  });
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9f3349 and 22826bb.

📒 Files selected for processing (3)
  • website/modules/asset/ui/src/js/formValidation.js (3 hunks)
  • website/modules/asset/ui/src/js/recaptchaValidation.js (1 hunks)
  • website/modules/asset/ui/src/js/recaptchaValidation.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • website/modules/asset/ui/src/js/formValidation.js
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.{js,jsx}`: Use 2 spaces for indentation Maximum line length: 80 characters...

**/*.{js,jsx}: Use 2 spaces for indentation
Maximum line length: 80 characters
Use semicolons at the end of statements
Use single quotes for strings
Add trailing commas in arrays, objects, etc.
No trailing spaces
Use consistent quote properties (either quote all properties or none)
Place line comments above code, not inline
Capitalize all comments
No inline comments
Maximum function length enforced (avoid excessive length)
Maximum depth: 4 levels
Maximum callback nesting: 3 levels
Maximum parameters: 5
Maximum statements per function: 50
Use function declarations with named functions, not function expressions
Always initialize variables at declaration
Maximum lines per file: 300
Use destructuring where possible
Minimum identifier length enforced (no single-letter variables)
No unused variables
No reassignment of function parameters
No invalid 'this' context
No duplicate object keys
No ternary operators (use if/else)
Maximum complexity: 15 (cognitive complexity)
No alerts or console logs
No debugger statements
No identical expressions in conditions
Use optimized regex patterns
Use Unicode regex patterns
No secrets in code
No unsanitized methods or properties (XSS prevention)
Sort imports alphabetically
No unresolved imports
No importing default from a module that doesn't have a default export
Always return in promise chains

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/recaptchaValidation.test.js
  • website/modules/asset/ui/src/js/recaptchaValidation.js
`**/*.js`: No missing imports in Node.js No missing require statements

**/*.js: No missing imports in Node.js
No missing require statements

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/recaptchaValidation.test.js
  • website/modules/asset/ui/src/js/recaptchaValidation.js
`**/*.test.{js,jsx}`: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring

**/*.test.{js,jsx}: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/asset/ui/src/js/recaptchaValidation.test.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
🧬 Code Graph Analysis (1)
website/modules/asset/ui/src/js/recaptchaValidation.test.js (2)
website/modules/asset/ui/src/js/recaptchaValidation.js (2)
  • require (1-1)
  • handleRecaptchaValueChange (3-17)
website/modules/asset/ui/src/js/formValidation.js (5)
  • require (1-1)
  • require (2-2)
  • require (3-3)
  • form (7-7)
  • form (15-15)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: e2e-tests
  • GitHub Check: unit-tests
  • GitHub Check: security-scan
  • GitHub Check: lint
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
website/modules/asset/ui/src/js/recaptchaValidation.js (1)

3-17: Function logic is correct and well-structured.

The handleRecaptchaValueChange function correctly handles the validation state updates and error message visibility. The optional parameter with default value provides good flexibility for testing.

Comment thread website/modules/asset/ui/src/js/recaptchaValidation.js Outdated
Comment thread website/modules/asset/ui/src/js/recaptchaValidation.js Outdated
Comment thread website/modules/asset/ui/src/js/recaptchaValidation.js Outdated
VitalyyP added 3 commits July 4, 2025 20:15
- Refactor verifyRecaptcha.js to use AbortController for timeout handling
- Add abort-controller dependency to package.json and package-lock.json
- Update reCAPTCHA token min length validation to 70 in validationSchemas.js
- Use 'x-forwarded-for' header if present for recaptcha remoteip
- Fallback to req.ip if header is missing
- Extract parameter validation to separate function
- Add explicit error for missing secret and remoteip
- Add tests for missing secret and remoteip cases

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22826bb and f5dd2ba.

⛔ Files ignored due to path filters (1)
  • website/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • website/modules/@apostrophecms/form/index.js (3 hunks)
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.js (1 hunks)
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js (1 hunks)
  • website/modules/asset/ui/src/js/validationSchemas.js (1 hunks)
  • website/package.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • website/modules/asset/ui/src/js/validationSchemas.js
  • website/modules/@apostrophecms/form/index.js
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.{js,jsx}`: Use 2 spaces for indentation Maximum line length: 80 characters...

**/*.{js,jsx}: Use 2 spaces for indentation
Maximum line length: 80 characters
Use semicolons at the end of statements
Use single quotes for strings
Add trailing commas in arrays, objects, etc.
No trailing spaces
Use consistent quote properties (either quote all properties or none)
Place line comments above code, not inline
Capitalize all comments
No inline comments
Maximum function length enforced (avoid excessive length)
Maximum depth: 4 levels
Maximum callback nesting: 3 levels
Maximum parameters: 5
Maximum statements per function: 50
Use function declarations with named functions, not function expressions
Always initialize variables at declaration
Maximum lines per file: 300
Use destructuring where possible
Minimum identifier length enforced (no single-letter variables)
No unused variables
No reassignment of function parameters
No invalid 'this' context
No duplicate object keys
No ternary operators (use if/else)
Maximum complexity: 15 (cognitive complexity)
No alerts or console logs
No debugger statements
No identical expressions in conditions
Use optimized regex patterns
Use Unicode regex patterns
No secrets in code
No unsanitized methods or properties (XSS prevention)
Sort imports alphabetically
No unresolved imports
No importing default from a module that doesn't have a default export
Always return in promise chains

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.js
`**/*.js`: No missing imports in Node.js No missing require statements

**/*.js: No missing imports in Node.js
No missing require statements

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js
  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.js
`**/*.test.{js,jsx}`: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring

**/*.test.{js,jsx}: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring

📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)

List of files the instruction was applied to:

  • website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/package.json (2)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/swipers.js : modules/asset/ui/src/swipers.js: relaxed import rules
website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js (1)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.test.{js,jsx} : Test files have relaxed rules for function length, statements, extraneous requires, and destructuring
website/modules/@apostrophecms/form/lib/verifyRecaptcha.js (2)
Learnt from: killev
PR: speedandfunction/website#21
File: scripts/merged-prs-last-24h.js:0-0
Timestamp: 2025-04-22T06:57:44.687Z
Learning: For console scripts like merged-prs-last-24h.js, the preferred approach is to allow fatal errors rather than implementing explicit error handling with try-catch blocks, as this makes errors more visible and provides complete stack traces for debugging.
Learnt from: killev
PR: speedandfunction/website#76
File: website/modules/@apostrophecms/form/index.js:154-157
Timestamp: 2025-05-11T21:14:53.791Z
Learning: The timestamp-based ID generation in the form submission handler (using Date.now().toString() in prepareSheetData method) is a known limitation but should not be flagged in PRs focused on code style or organization. This is acceptable for now and would be addressed separately if needed.
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: e2e-tests
  • GitHub Check: lint
  • GitHub Check: security-scan
  • GitHub Check: unit-tests
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (actions)
🔇 Additional comments (6)
website/package.json (1)

44-44: Dependency appropriately chosen for timeout handling.

The abort-controller dependency is correctly added to support timeout functionality with node-fetch v2.

website/modules/@apostrophecms/form/lib/verifyRecaptcha.test.js (1)

1-82: Comprehensive test coverage for reCAPTCHA verification.

The test suite covers all critical scenarios including parameter validation, API response handling, and error cases. The structure is clean and follows Jest best practices with proper mocking.

website/modules/@apostrophecms/form/lib/verifyRecaptcha.js (4)

4-15: Well-structured parameter validation.

The validation function properly checks all required parameters and returns appropriate error messages. The separation of validation logic makes the code more maintainable.


17-60: Robust HTTP request implementation with proper timeout handling.

The request function correctly implements:

  • AbortController for reliable timeout handling
  • Proper error handling for network issues
  • Appropriate response validation
  • Clean error messages for different failure scenarios

62-68: Clean main function with proper delegation.

The main verifyRecaptcha function appropriately delegates to validation and request functions, maintaining clean separation of concerns.


28-28: Google reCAPTCHA endpoint is correct

The existing URL (https://www.google.com/recaptcha/api/siteverify) matches the official Google reCAPTCHA v2 verification endpoint, so no update is needed.

Comment thread website/package.json Outdated
@yuramax
yuramax dismissed stale reviews from VitalyyP, Anton-88, and coderabbitai[bot] via 9538251 July 5, 2025 16:12
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 5, 2025
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 5, 2025
…analysis - SonarQube rule can be ignored for recaptcha-script.html

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
sonar-project.properties (1)

23-24: Duplicate & overly specific exclusions warrant a quick cleanup

  1. website/coverage/** is already excluded by sonar.coverage.exclusions on line 8, so repeating it here is redundant noise.
  2. The single-file exclusion for recaptcha-script.html is fine, but since the entire website/modules/** tree is already excluded from coverage (line 11) you might consider adding the same path to sonar.exclusions instead of whitelisting files piecemeal. That keeps the two exclusion blocks in sync and avoids future drift.
-  website/coverage/**,\
-  website/modules/@apostrophecms/form-widget/views/recaptcha-script.html
+  website/modules/**,\
+  # keep any one-off overrides below this line if needed
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1)

1-1: Add CSP-friendly attributes for safer third-party script loading

Consider adding nonce and crossorigin="anonymous" so sites with a strict Content-Security-Policy don’t have to maintain an inline-script exception for reCAPTCHA:

-<script src="https://www.google.com/recaptcha/api.js" async defer></script>
+<script
+  src="https://www.google.com/recaptcha/api.js"
+  async
+  defer
+  crossorigin="anonymous"
+  nonce="{{ apos.asset.nonce() }}"
+></script>

If CSP isn’t enforced yet you can ignore, but adding it now is a cheap hardening win.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39f14a6 and 4171c1a.

📒 Files selected for processing (3)
  • sonar-project.properties (1 hunks)
  • website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1 hunks)
  • website/modules/@apostrophecms/form-widget/views/widget.html (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • website/modules/@apostrophecms/form-widget/views/widget.html
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: VitalyyP
PR: speedandfunction/website#197
File: website/modules/asset/ui/src/js/recaptchaValidation.js:30-35
Timestamp: 2025-07-04T17:37:05.605Z
Learning: The recaptchaValidation.js file in the speedandfunction/website repository has proper memory leak prevention with interval cleanup, event listener removal, and DOM removal detection. The cleanup is handled automatically when elements are removed from the DOM and through a returned cleanup function.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
sonar-project.properties (2)
Learnt from: killev
PR: speedandfunction/website#4
File: sonar-project.properties:0-0
Timestamp: 2025-04-13T14:32:00.114Z
Learning: In the CI pipeline for this project, test coverage reports are generated in the website/coverage directory but are then downloaded as artifacts to the root working directory before the SonarQube scan, making sonar.javascript.lcov.reportPaths=lcov.info the correct configuration in sonar-project.properties.
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.{js,jsx} : No secrets in code
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1)
Learnt from: VitalyyP
PR: speedandfunction/website#197
File: website/modules/asset/ui/src/js/recaptchaValidation.js:30-35
Timestamp: 2025-07-04T17:37:05.605Z
Learning: The recaptchaValidation.js file in the speedandfunction/website repository has proper memory leak prevention with interval cleanup, event listener removal, and DOM removal detection. The cleanup is handled automatically when elements are removed from the DOM and through a returned cleanup function.
🪛 HTMLHint (1.5.0)
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html

[error] 1-1: Doctype must be declared before any non-comment content.

(doctype-first)

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: e2e-tests
  • GitHub Check: lint
  • GitHub Check: unit-tests
  • GitHub Check: security-scan
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1)

1-1: doctype-first HTMLHint warning is a false positive for Apostrophe partials

This template is an HTML fragment injected into a larger page, so omitting <!DOCTYPE html> is intentional. Suppress the rule for the views/ directory in .htmlhintrc (or via a file-level comment) to keep pipelines green without disabling it globally.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 5, 2025
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 5, 2025
- Move reCAPTCHA script from widget.html to recaptcha-script.html

- Update widget.html to use include instead of inline script

- Add recaptcha-script.html to sonar exclusions

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1)

1-1: Consider loading the reCAPTCHA script once and add CSP/SRI hardening

Placing this <script> in a per-widget partial means every form instance inserts the tag, causing redundant network requests and making CSP rollout harder.
Move the tag to a project-level asset (e.g. apos.asset.push('script', …, { when: 'always' })) or wrap the partial in an apos.util.once guard, and add a CSP nonce or SRI hash plus crossorigin="anonymous" for extra security.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86d6b41 and 456194b.

📒 Files selected for processing (3)
  • sonar-project.properties (1 hunks)
  • website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1 hunks)
  • website/modules/@apostrophecms/form-widget/views/widget.html (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • sonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (1)
  • website/modules/@apostrophecms/form-widget/views/widget.html
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: VitalyyP
PR: speedandfunction/website#197
File: website/modules/asset/ui/src/js/recaptchaValidation.js:30-35
Timestamp: 2025-07-04T17:37:05.605Z
Learning: The recaptchaValidation.js file in the speedandfunction/website repository has proper memory leak prevention with interval cleanup, event listener removal, and DOM removal detection. The cleanup is handled automatically when elements are removed from the DOM and through a returned cleanup function.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1)
Learnt from: VitalyyP
PR: speedandfunction/website#197
File: website/modules/asset/ui/src/js/recaptchaValidation.js:30-35
Timestamp: 2025-07-04T17:37:05.605Z
Learning: The recaptchaValidation.js file in the speedandfunction/website repository has proper memory leak prevention with interval cleanup, event listener removal, and DOM removal detection. The cleanup is handled automatically when elements are removed from the DOM and through a returned cleanup function.
🪛 HTMLHint (1.5.0)
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html

[error] 1-1: Doctype must be declared before any non-comment content.

(doctype-first)

🔇 Additional comments (1)
website/modules/@apostrophecms/form-widget/views/recaptcha-script.html (1)

1-1: Ignore “doctype-first” HTMLHint warning

This file is a fragment, not a standalone HTML document, so omitting the <!doctype> is expected.

@sonarqubecloud

sonarqubecloud Bot commented Jul 5, 2025

Copy link
Copy Markdown

@yuramax
yuramax requested review from Anton-88, VitalyyP and yuramax July 5, 2025 20:50

@yuramax yuramax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@Anton-88 Anton-88 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@VitalyyP VitalyyP left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@killev killev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@killev
killev merged commit 336cb19 into main Jul 8, 2025
12 checks passed
@killev
killev deleted the 659-admin-recaptcha-config branch July 8, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants